-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add port-sniffer by Xsorter #32
base: master
Are you sure you want to change the base?
Conversation
const range = findPortsRange(); | ||
|
||
const ports = { | ||
firstPort: +range[0], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont need every time add
+` for converting type to number.
Just guarantee, that findPortsRange will return numbers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for advice, changed type in function's return
socket.on('connect', () => { | ||
clearTimeout(time); | ||
socket.destroy(); | ||
process.stdout.write('.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont use process.stdout.write
Prefet to use console.log / warn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, changed to console.log
const showResult = host => { | ||
if (args.help) { | ||
process.stdout.write(messages().help); | ||
process.exit(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad practice to fail app with such aggressive methods :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understand, i'll just return false than
process.stdout.write(messages().help); | ||
process.exit(1); | ||
} | ||
if (!args.host) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no hosts -> let's ask again about hosts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this message
process.stdout.write(messages().noHost); | ||
process.exit(1); | ||
} else { | ||
openedPortCheck(host, ports.firstPort, function nextIteration () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This recursion is not effective due to huge memory usage, slowness, and other reasons.
I propose to you rewrite this to one of the ways:
- Async Iterations (can be paralleled like 10+ in one batch iterations, looks how https://caolan.github.io/async/v3/docs.html#mapLimit works)
- Generator (if use 12+ nodejs -> async generator)
- If you want recursion, let it be optimized, like tails recursion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, i changed recursion with mapLimit
}; | ||
|
||
ipLookup() | ||
.then(res => showResult(res)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can use console.table to write better output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
I've read
./README.md
and./CODE_QUALITY.md
carefully.The code is checked by
yarn run lint:js
and linter reported no errors.The code is submitted in its own sub-directory and in a dedicated feature branch.
Please, review.